Java: Convert all collection and array steps from taint flow to value flow. by aschackmull · Pull Request #5751 · github/codeql

aschackmull · 2021-04-22T14:59:41Z

This converts all collection and array flow steps to use precise value flow rather than taint flow. This means that read-steps must match up with preceding store-steps, and that the tracked object can be followed more precisely through collections and arrays, both in terms of its type and in terms of the flow being applicable in pure DataFlow::Configurations. This allows more flow for value-based data flow, which is relevant for both DataFlow::Configurations and the subpaths of TaintTracking::Configuration that are restricted to value flow (i.e. when tracking through fields), and more precision in taint tracking compared to simply marking a collection or array as tainted.

As an example, FP flow like the following is no longer reported:

String taintedString = taint;
Map<K, Object> map;
map.put(k, taintedString);
Object obj = map.get(..);
byte[] bytes = (byte[])obj;
sink(bytes);

Collection and array read steps are still also treated as taint steps to support a suitable interpretation of a "tainted collection" or "tainted array".
In addition, a temporary measure to provide a large degree of backwards compatibility with ad-hoc taint flows, a set of configuration-dependent store-as-taint steps are added to support sinks and steps that read from collections/arrays.

hvitved · 2021-04-22T17:54:11Z

+        "java.util.stream;SpinedBuffer;true;copyInto;(Object[],int);;Element of Argument[-1];ArrayElement of Argument[0];value",
+        "java.util.stream;SpinedBuffer<>$OfPrimitive;true;copyInto;(Object,int);;Element of Argument[-1];ArrayElement of Argument[0];value",
+        "java.util;List;false;copyOf;(Collection);;Element of Argument[0];Element of ReturnValue;value",
+        "java.util;List;false;of;(Object[]);;Element of Argument[0];Element of ReturnValue;value",


OOI, what does the empty of in ;of; mean?

That's just the name of the varargs method List::of and not related to the DSL.

smowton

Reviewed up to but excluding NavigableMap

smowton · 2021-04-23T10:28:42Z

-    sink.(DataFlow::ImplicitVarargsArray).getCall() = arg.getCall()
+  exists(Type elemType |
+    arrayReadStep(src, sink, elemType) and
+    not elemType instanceof PrimitiveType and


Comment on why these exclusions are made here?

Because numbers usually don't carry taint. I.e. by default we'll say that an int[] can be tainted without implying that the individual elements are tainted, whereas when a MyObject[] is tainted then we'll take that to mean that all of the contained MyObjects are tainted as well.

Oh sure, but I'm surprised to find this changing as part of this work? Didn't we previously permit taint to travel from x to x[0] even if x was an int[]?

You could say that we're adding a taint step here rather than restricting one, since the general x to x[0] step is now considered an array read step that's expected to be matched by an array store step.

smowton · 2021-04-23T10:30:48Z

+        "java.util;Map<>$Entry;true;setValue;;;MapValue of Argument[-1];ReturnValue;value",
+        "java.util;Map<>$Entry;true;setValue;;;Argument[0];MapValue of Argument[-1];value",


Will this pair work as expected? (the return is the old value)

Yes, when we're targeting an argument as the output then it actually means the corresponding post-update node. So there's no overlap between these two lines, and thus should work just fine (with the obvious exception that the update isn't reflected back to the underlying map, which is a lot harder to do, since that's moving into aliasing territory).

smowton · 2021-04-23T10:35:17Z

+        "java.util;Map;true;get;;;MapValue of Argument[-1];ReturnValue;value",
+        "java.util;Map;true;getOrDefault;;;MapValue of Argument[-1];ReturnValue;value",
+        "java.util;Map;true;getOrDefault;;;Argument[1];ReturnValue;value",
+        "java.util;Map;true;put;;;MapValue of Argument[-1];ReturnValue;value",


Same here I suppose -- the old value is returned, but we're presumably going to wire these steps together and alias the new one

smowton · 2021-04-23T10:37:47Z

+        "java.util;Map;true;entrySet;;;MapKey of Argument[-1];MapKey of Element of ReturnValue;value",
+        "java.util;Map;true;get;;;MapValue of Argument[-1];ReturnValue;value",
+        "java.util;Map;true;getOrDefault;;;MapValue of Argument[-1];ReturnValue;value",
+        "java.util;Map;true;getOrDefault;;;Argument[1];ReturnValue;value",


Missing keySet?

Yep. I'll add it.

smowton · 2021-04-23T10:39:48Z

+        "java.util;List;true;set;;;Argument[1];Element of Argument[-1];value",
+        "java.util;List;true;subList;;;Element of Argument[-1];Element of ReturnValue;value",
+        "java.util;List;true;add;(int,Object);;Argument[1];Element of Argument[-1];value",
+        "java.util;List;true;set;(int,Object);;Argument[1];Element of Argument[-1];value",


Redundant with line 142

good catch, will remove

smowton · 2021-04-23T10:42:43Z

+        "java.util;Queue;true;poll;();;Element of Argument[-1];ReturnValue;value",
+        "java.util;Queue;true;remove;();;Element of Argument[-1];ReturnValue;value",
+        "java.util;Queue;true;offer;(Object);;Argument[0];Element of Argument[-1];value",
+        "java.util;Deque;true;descendingIterator;();;Element of Argument[-1];ReturnValue;value",


Other iterators seem to use Element of ...

yeah, that's a typo...

smowton

Reviewed the rest.

I guess now is the time to consider whether all these parallel routings of MapKeys and MapValues is enough to make us wish maps were composed of Elements which themselves have Keys and Values.

smowton · 2021-04-23T14:34:25Z

+        "java.util;Arrays;false;fill;(char[],char);;Argument[1];ArrayElement of Argument[0];value",
+        "java.util;Arrays;false;fill;(short[],int,int,short);;Argument[3];ArrayElement of Argument[0];value",
+        "java.util;Arrays;false;fill;(short[],short);;Argument[1];ArrayElement of Argument[0];value",
+        "java.util;Arrays;false;fill;(int[],int,int,int);;Argument[3];ArrayElement of Argument[0];value",


I note lots of primitive propagation steps that are ruled out elsewhere?

Where are they ruled out?

I thought these would be ruled out by the numeric-type exclusions discussed in the other thread, but perhaps not?

No. The bulk of this work is about adding value-flow through arrays and collections by modelling the steps as proper read- and store steps (aka as field flow). This means that the PR removes those steps from the default set of taint steps, and instead adds them as general read- and store steps that use the synthetic field ArrayElement. The array read step with the type exclusions in TaintTrackingUtil.qll is a taint step and not a read step.

aschackmull · 2021-05-04T13:30:17Z

https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1375/

aschackmull · 2021-05-05T08:46:34Z

Jdk11 has still not finished its query evaluations, so performance is likely not looking very good.

aschackmull · 2021-05-05T09:57:58Z

Jdk11 has still not finished its query evaluations, so performance is likely not looking very good

Looks like I accidentally wrote a big cartesian product. Oops.

aschackmull · 2021-05-05T11:07:28Z

~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1377/~~
https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1378/

aschackmull · 2021-05-06T08:25:48Z

The full combination of precise collection read/store steps and backwards compatible taint steps appears to be too costly. Let's see if a suitable middle ground works.
~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1380/~~ (skipping guava as that doesn't seem to build)
~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1381/~~
https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1383/

aschackmull · 2021-05-12T08:40:02Z

Config-dependent store-as-taint: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1388/

aschackmull · 2021-05-18T09:19:15Z

Test tentative performance tweak: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1403/

aschackmull · 2021-05-19T12:31:56Z

I've btw. checked the fixed results on geode. They are of the following form:

String taintedString = taint;
Map<K, Object> map;
map.put(k, taintedString);
Object obj = map.get(..);
byte[] bytes = (byte[])obj;
sink(bytes);

I've left out the irrelevant parts (some paths are > 100 steps, and the above steps are separated by many calls). With collection flow as taint we get this sort of FP flow, but with precise collection flow we are able to prune the path based on type information, as we are able to retain the type String inside the map, and when it's read back out we can see that it's incompatible with the cast to byte[]. 🎉

yo-h · 2021-05-19T17:14:05Z

With collection flow as taint we get this sort of FP flow, but with precise collection flow we are able to prune the path based on type information

Nice!

JLLeitschuh · 2021-05-21T15:11:43Z

+  private import semmle.code.java.dataflow.TaintTracking2
+
+  private class StoreTaintConfig extends TaintTracking::Configuration {
+    StoreTaintConfig() { this instanceof TaintTracking::Configuration or none() }


This is somewhat unusual, could you explain what's going on here? How does this not cause collisions with other implementations of TaintTracking::Configuration actually used in the query?

This is indeed unusual 😉 And yes, it collides with every other configuration. It's a hack to globally modify all other configurations with code that is itself configuration-dependent.

Why use this vs implementing AdditionalTaintStep? Isn't the API for isAdditionalTaintStep exactly the same and doesn't creating implementations of AdditionalTaintStep have exactly the same effect?

No. The Configuration::isAdditionalTaintStep predicate is configuration-specific, whereas AdditionalTaintStep is configuration-independent (i.e. global and applies to all configs).

Why would this collection data flow only be applied to TaintTracking and TaintTracking2? Why not all of them?

There are only two copies TaintTracking at the moment, so this should be all of them.

Besides, this is a temporary measure, since it is quite costly in terms of performance.

smowton · 2021-06-01T13:42:26Z

  containerUpdateStep(n1, n2)
 }
+
+predicate arrayStoreStep(Node node1, Node node2) {


Make private or document

Hmm, why wasn't this caught by the PR check?

smowton · 2021-06-01T13:42:35Z

+  )
+}
+
+predicate arrayReadStep(Node node1, Node node2, Type elemType) {


Make private or document

smowton · 2021-06-01T13:42:39Z

+  )
+}
+
+predicate collectionReadStep(Node node1, Node node2) {


Make private or document

smowton · 2021-06-01T13:44:49Z


 import Cached

+private module StoreTaintSteps {


Document this for the benefit of external readers who ought to know this is a stop-gap / temporary measure (and what it intends to do).

This reverts commit 1c081ee.

github-actions Bot added the Java label Apr 22, 2021

hvitved reviewed Apr 22, 2021

View reviewed changes

smowton reviewed Apr 23, 2021

View reviewed changes

intrigus-lgtm mentioned this pull request Apr 26, 2021

Java: Add/improve insecure trustmanager query #4879

Merged

aschackmull force-pushed the java/collection-flow branch from ac5a157 to febc063 Compare May 5, 2021 13:07

aschackmull force-pushed the java/collection-flow branch from 9312507 to d828e24 Compare May 6, 2021 11:28

github-actions Bot added C# C++ Python labels May 18, 2021

aschackmull force-pushed the java/collection-flow branch 2 times, most recently from bb377e3 to cd87559 Compare May 19, 2021 11:30

aschackmull force-pushed the java/collection-flow branch from cd87559 to e21063a Compare May 20, 2021 10:56

JLLeitschuh reviewed May 21, 2021

View reviewed changes

aschackmull added 7 commits June 1, 2021 11:47

Java: Switch array steps and one containerstep.

1001dd8

Java: Fix bug in matching generic signatures.

ffd52bb

Java: Add container flow models.

3b6cef4

Java: Remove container taint steps.

9e313d0

Java: Update some models.

3f538e7

Java: Allow <> in types for now.

2f087e1

Java: Add read-as-taint and config-dependent store-as-taint.

a40880a

aschackmull added 4 commits June 1, 2021 11:47

Java: Update qltests.

43d1b0a

Java: Add collection flow test.

901996f

Java: Remove deprecated tests.

dbe352f

Java: Minor model fix.

fc913e7

aschackmull force-pushed the java/collection-flow branch from c5d8278 to fc913e7 Compare June 1, 2021 09:48

Java: Update coverage.

1c081ee

aschackmull force-pushed the java/collection-flow branch from cb1f31a to 1c081ee Compare June 1, 2021 12:00

Java: Add change note.

922b421

aschackmull marked this pull request as ready for review June 1, 2021 12:34

aschackmull requested a review from a team as a code owner June 1, 2021 12:34

github-actions Bot added the documentation label Jun 1, 2021

smowton reviewed Jun 1, 2021

View reviewed changes

aschackmull added 2 commits June 1, 2021 16:09

Java: More qldoc.

650c4f1

Revert "Java: Update coverage."

e86c534

This reverts commit 1c081ee.

yo-h approved these changes Jun 3, 2021

View reviewed changes

aschackmull merged commit bd9e3d0 into github:main Jun 3, 2021

aschackmull deleted the java/collection-flow branch June 3, 2021 13:29

atorralba mentioned this pull request Jun 7, 2021

Java: Add Map key-read-steps as local additional taint steps #6029

Merged

		"java.util;Map<>$Entry;true;setValue;;;MapValue of Argument[-1];ReturnValue;value",
		"java.util;Map<>$Entry;true;setValue;;;Argument[0];MapValue of Argument[-1];value",

Conversation

aschackmull commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented May 4, 2021

Uh oh!

aschackmull commented May 5, 2021

Uh oh!

aschackmull commented May 5, 2021

Uh oh!

aschackmull commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented May 12, 2021

Uh oh!

aschackmull commented May 18, 2021

Uh oh!

aschackmull commented May 19, 2021

Uh oh!

yo-h commented May 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aschackmull commented Apr 22, 2021 •

edited

Loading

aschackmull commented May 5, 2021 •

edited

Loading

aschackmull commented May 6, 2021 •

edited

Loading